fix: empty ID for derived accounts leading to state saving failures#206
fix: empty ID for derived accounts leading to state saving failures#206ulissesferreira wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
You will notice that the findByIds method incorrectly returned null as a possible type. That made no sense since when you can't find something by IDs you simply return an empty array instead of an array with the results... Changed that.
| }); | ||
|
|
||
| return { | ||
| id: '', |
There was a problem hiding this comment.
What's the benefit of having the ID generated separately from the rest of the account object?
|
|
||
| const mappedTransactions = TransactionMapper.mapTransactions({ | ||
| scope, | ||
| account: account as TronKeyringAccount, |
| const privateKeyHex = node.privateKey.slice(2); | ||
|
|
||
| // Derive address from public key (cheaper than from private key) | ||
| const ethAddress = computeAddress(node.publicKey); |
There was a problem hiding this comment.
It's not an eth addres... It's in hexadecimal format. Let's call things by their appropriate names.
| }); | ||
|
|
||
| return { | ||
| id: '', |
There was a problem hiding this comment.
Very evil... The source of the bug basically... Was a clear sign that separation was needed here.
| }; | ||
| } | ||
|
|
||
| async create( |
There was a problem hiding this comment.
This was something I always hated. Why pass in an ID when creating the account? The answer is we wanted to be able to rollback account creations from the handler method in case we could not emit them here. So we stored the ID and caught errors and deleted the account from the state if it occurred. That is very convoluted. Simply move the logic to the service, the way it should be.
2 participants
Explanation
The existing codebase conflates derived accounts, which haven't been persisted, with accounts that are already part of the Tron Snap. Not being strict about this lead to the bug reported here. We can do better, creating a more clear separation and favoring that over being "smart" and using less functions.
This PR:
References
Closes NEB-555
Closes MetaMask/metamask-extension#40098
Checklist